Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

deps: bump go-getter to 1.5.4 #10778

Merged
merged 1 commit into from
Jun 17, 2021
Merged

deps: bump go-getter to 1.5.4 #10778

merged 1 commit into from
Jun 17, 2021

Conversation

tgross
Copy link
Member

@tgross tgross commented Jun 17, 2021 •
edited
Loading

Fixes #10438

@tgross tgross force-pushed the deps-go-getter-1.5.4 branch from 7c710c0 to fbf9acd Compare June 17, 2021 18:13
@vercel vercel bot temporarily deployed to Preview – nomad June 17, 2021 18:13 Inactive
@tgross tgross requested review from picatz, notnoop and shoenig June 17, 2021 18:13
@tgross tgross marked this pull request as ready for review June 17, 2021 18:14
@tgross tgross added this to the 1.1.2 milestone Jun 17, 2021
@tgross tgross mentioned this pull request Jun 17, 2021
Closed
notnoop
notnoop approved these changes Jun 17, 2021
View reviewed changes
Copy link
Contributor

@notnoop notnoop left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

The line between improvement vs bug seems very fuzzy to me. Is it worth backporting to 1.0.X? Though, dependency changes curry a bit of risk so it makes sense not to backport if we don't have to.

vendor/github.com/hashicorp/go-cleanhttp/cleanhttp.go Show resolved Hide resolved
@tgross tgross force-pushed the deps-go-getter-1.5.4 branch from fbf9acd to f189712 Compare June 17, 2021 19:33
@vercel vercel bot temporarily deployed to Preview – nomad June 17, 2021 19:33 Inactive
@tgross tgross force-pushed the deps-go-getter-1.5.4 branch from f189712 to b8dfdfd Compare June 17, 2021 19:39
@vercel vercel bot temporarily deployed to Preview – nomad June 17, 2021 19:39 Inactive
@tgross
Copy link
Member Author

tgross commented Jun 17, 2021

The line between improvement vs bug seems very fuzzy to me. Is it worth backporting to 1.0.X? Though, dependency changes curry a bit of risk so it makes sense not to backport if we don't have to.

I don't have a strong opinion on that but seeing as how this was Support-facing, we should probably plan on backporting. I've modified the changelog based on that.

@tgross tgross added stage/needs-backporting theme/dependencies Pull requests that update a dependency file labels Jun 17, 2021
picatz
picatz approved these changes Jun 17, 2021
View reviewed changes
vendor/github.com/hashicorp/go-getter/client.go
@@ -289,7 +298,7 @@ func (c *Client) Get() error {
// if we're specifying a subdir.
err := g.Get(dst, u)
if err != nil {
err = fmt.Errorf("error downloading '%s': %s", src, err)
err = fmt.Errorf("error downloading '%s': %s", u.Redacted(), err)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW, this line is particularly interesting (the optional insecure config aside). 🤔

My thinking being that this method would help certain, but not all cases where credentials are included as part of the URL. Not totally sure how I feel about it, but not a reason to block the PR.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be nice to introduce in other use cases in go-getter itself, for sure.

@tgross tgross merged commit 77f6ecb into main Jun 17, 2021
@tgross tgross deleted the deps-go-getter-1.5.4 branch June 17, 2021 20:30
notnoop pushed a commit that referenced this pull request Jun 22, 2021
@tgross
deps: bump go-getter to 1.5.4 (#10778)
d471302
notnoop pushed a commit that referenced this pull request Jul 28, 2021
api: revert to defaulting to http/1
0c3c559 
PR #10778 incidentally changed the api http client to connect with
HTTP/2 first. However, the websocket libraries used in `alloc exec`
features don't handle http/2 well, and don't downgrade to http/1
gracefully.

Given that the switch is incidental, and not requested by users.
Furthermore, api consumers can opt-in to forcing http/2 by setting
custom http clients.
@notnoop notnoop mentioned this pull request Jul 28, 2021
Merged
notnoop pushed a commit that referenced this pull request Jul 28, 2021
api: revert to defaulting to http/1 (#10958)
d3835a1 
* api: revert to defaulting to http/1

PR #10778 incidentally changed the api http client to connect with
HTTP/2 first. However, the websocket libraries used in `alloc exec`
features don't handle http/2 well, and don't downgrade to http/1
gracefully.

Given that the switch is incidental, and not requested by users.
Furthermore, api consumers can opt-in to forcing http/2 by setting
custom http clients.

Fixes #10922
@github-actions
Copy link

I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 15, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@picatz picatz picatz approved these changes

@notnoop notnoop notnoop approved these changes

@shoenig shoenig Awaiting requested review from shoenig

Assignees
No one assigned
Labels
theme/dependencies Pull requests that update a dependency file
Projects
None yet
Milestone
1.1.2
Development

Successfully merging this pull request may close these issues.

artifact block doesn't detect 5 part vhosted-style s3 buckets
4 participants
@tgross @notnoop @picatz @schmichael